-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
refactor(core): add generic utilities for resolving value-or-function patterns, replace specialized resolveStaleTime
and resolveEnabled
#9212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
resolveStaleTime
and resolveEnabled
|
Command | Status | Duration | Result |
---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 2m 22s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 22s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-09-01 11:11:08
UTC
df0249f
to
2421a38
Compare
… patterns, replace specialized `resolveStaleTime` and `resolveEnabled` This commit introduces `isFunctionVariant()` and `resolveValueOrFunction()` utilities to replace repetitive `typeof === 'function'` checks throughout the codebase, consolidating the common "value or function that computes value" pattern.
409a222
to
b28e950
Compare
… NonFunctionGuard This simplification is possible due to the introduction of generic runtime utilities that handle value-or-function resolution. The new `resolveValueOrFunction()` utility handles the distinction between direct values and functions at runtime with proper type safety, eliminating the need for complex type-level guards.
b28e950
to
cd59cc4
Compare
9d15984
to
427c977
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally agree with the direction, please have a look at the comments though
@@ -58,7 +58,8 @@ | |||
"@angular/platform-browser-dynamic": "^20.0.0", | |||
"@tanstack/angular-query-experimental": "workspace:*", | |||
"eslint-plugin-jsdoc": "^50.5.0", | |||
"npm-run-all2": "^5.0.0" | |||
"npm-run-all2": "^5.0.0", | |||
"tsup": "^8.4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was adding tsup
necessary here? I don’t think angular uses tsup
for bundling 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed! In earlier commits, the lack of tsup was blocking builds.
): T { | ||
return isFunctionVariant(value) ? value(...args) : value | ||
} | ||
|
||
export function functionalUpdate<TInput, TOutput>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we use resolveValueOrFunction
here as well, as this also uses a typeof updater === 'function'
check ?
packages/query-core/src/utils.ts
Outdated
* const delay = resolveValueOrFunction(retryDelay, failureCount, error) | ||
* ``` | ||
*/ | ||
export function resolveValueOrFunction<T, TArgs extends Array<any>>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a name like resolveOption
would be descriptive enough
hm, the failing pipeline shows a lot of type-errors |
Remove specialized resolveEnabled function and consolidate functionality into the generic resolveOption utility. Both functions performed identical operations but resolveEnabled was type-specific to query enabled state. - Remove resolveEnabled function from utils.ts - Update queryObserver.ts to use resolveOption instead - Remove unused Enabled type import - Maintain identical functionality and type safety
it’s been two months, there’s still a failing pipeline and conflicts. please let me know if you intend to finish this PR, because it doesn’t look like it’s close to working. I’ll close it otherwise. |
Thanks for the ping! Will update it and get back to you by EOD. |
WalkthroughReplaced specialized per-option resolvers with a single exported resolveOption across query-core; updated query, observer, client, retryer, utils, and types to use it. Removed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Consumer
participant QO as QueryObserver
participant Q as Query
participant QC as QueryClient
participant U as utils.resolveOption
participant R as Retryer
C->>QO: setOptions(options)
QO->>U: resolveOption(options.enabled, query)
U-->>QO: isEnabled
QO->>U: resolveOption(options.staleTime, query)
U-->>QO: staleTime
QO->>Q: request initial/placeholder data
Q->>U: resolveOption(options.initialData, query)
U-->>Q: initialData
alt Needs fetch
QO->>QC: fetchQuery(queryKey, options)
QC->>U: resolveOption(options.staleTime, query)
U-->>QC: staleTime
QC->>R: start retryer
R->>U: resolveOption(options.retryDelay, failureCount, error)
U-->>R: delay
R-->>QC: result/error
QC-->>QO: result
QO-->>C: notify result
else No fetch
QO-->>C: notify cached result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-core/src/types.ts (1)
169-177
: PlaceholderDataFunction type is now internal but still referenced in public APIThe
PlaceholderDataFunction
type has been changed from exported to internal (noexport
keyword), but it's still being used in the publicQueryObserverOptions
interface at lines 424-426. This will cause TypeScript compilation errors for consumers who need to understand or reference this type.Apply this diff to fix the type visibility issue:
-type PlaceholderDataFunction< +export type PlaceholderDataFunction< TQueryFnData = unknown, TError = DefaultError, TQueryData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, > = ( previousData: TQueryData | undefined, previousQuery: Query<TQueryFnData, TError, TQueryData, TQueryKey> | undefined, ) => TQueryData | undefinedAlternatively, if the intent is to keep it internal, consider inlining the function signature directly in the
placeholderData
property definition.
♻️ Duplicate comments (1)
packages/query-core/src/utils.ts (1)
168-175
: Use resolveOption here for consistency (duplicate of earlier feedback).This is the same “value or updater function” pattern. Reusing
resolveOption
or the localisFunctionVariant
keeps the file consistent and DRY.Apply this diff:
export function functionalUpdate<TInput, TOutput>( updater: Updater<TInput, TOutput>, input: TInput, ): TOutput { - return typeof updater === 'function' - ? (updater as (_: TInput) => TOutput)(input) - : updater + return isFunctionVariant<Updater<TInput, TOutput>, [TInput]>(updater) + ? (updater as (_: TInput) => TOutput)(input) + : (updater as TOutput) }Note: This still shares the same ambiguity if
TOutput
is itself a function. If you adoptNonFunctionGuard
forresolveOption
, consider mirroring that here for true parity.
🧹 Nitpick comments (2)
packages/query-core/src/query.ts (1)
692-698
: Consider adding type safety for InitialDataFunction patternWhile the
resolveOption
utility handles the function-or-value pattern generically, the removal of theInitialDataFunction
type means we lose some type safety. The current implementation will accept any function, not just zero-argument functions.Consider adding a type constraint to ensure
initialData
andinitialDataUpdatedAt
functions don't accidentally accept parameters:- const data = resolveOption(options.initialData) + const data = resolveOption(options.initialData as TData | (() => TData | undefined)) const hasData = data !== undefined const initialDataUpdatedAt = hasData - ? resolveOption(options.initialDataUpdatedAt) + ? resolveOption(options.initialDataUpdatedAt as number | (() => number | undefined) | undefined) : 0packages/query-core/src/utils.ts (1)
80-116
: Tighten the type guard: return a function typed to T, not any.Returning
any
from the predicate unnecessarily weakens type inference in downstream callers (includingresolveOption
). Have the predicate assert(...args: TArgs) => T
and preferunknown[]
overArray<any>
.Apply this diff:
-function isFunctionVariant<T, TArgs extends Array<any> = []>( - value: T | ((...args: TArgs) => any), -): value is (...args: TArgs) => any { +function isFunctionVariant<T, TArgs extends unknown[] = []>( + value: T | ((...args: TArgs) => T), +): value is (...args: TArgs) => T { return typeof value === 'function' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/query-core/src/query.ts
(4 hunks)packages/query-core/src/queryClient.ts
(3 hunks)packages/query-core/src/queryObserver.ts
(14 hunks)packages/query-core/src/retryer.ts
(2 hunks)packages/query-core/src/types.ts
(2 hunks)packages/query-core/src/utils.ts
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/query-core/src/queryClient.ts (2)
packages/query-core/src/queryObserver.ts (2)
query
(680-697)resolveOption
(364-368)packages/query-core/src/utils.ts (1)
resolveOption
(161-166)
packages/query-core/src/retryer.ts (2)
packages/query-core/src/queryObserver.ts (1)
resolveOption
(364-368)packages/query-core/src/utils.ts (1)
resolveOption
(161-166)
packages/query-core/src/queryObserver.ts (1)
packages/query-core/src/utils.ts (1)
resolveOption
(161-166)
packages/query-core/src/query.ts (2)
packages/query-core/src/queryObserver.ts (1)
resolveOption
(364-368)packages/query-core/src/utils.ts (1)
resolveOption
(161-166)
packages/query-core/src/utils.ts (1)
packages/query-core/src/queryObserver.ts (1)
resolveOption
(364-368)
🔇 Additional comments (14)
packages/query-core/src/queryClient.ts (2)
158-159
: LGTM! Clean replacement of resolveStaleTime with resolveOptionThe change correctly replaces the specialized
resolveStaleTime
with the genericresolveOption
utility, maintaining the same functionality while improving code consistency.
365-365
: LGTM! Consistent use of resolveOptionThe replacement maintains the same behavior while using the unified
resolveOption
utility.packages/query-core/src/retryer.ts (1)
169-169
: LGTM! Simplified delay resolutionThe change correctly replaces the manual conditional logic with the generic
resolveOption
utility, making the code more concise and consistent with the rest of the codebase.packages/query-core/src/query.ts (2)
257-257
: LGTM! Consistent enabled option resolutionThe change correctly replaces
resolveEnabled
with the genericresolveOption
utility.
276-276
: LGTM! Consistent staleTime resolutionThe change correctly replaces
resolveStaleTime
with the genericresolveOption
utility.packages/query-core/src/queryObserver.ts (7)
158-159
: LGTM! Proper validation with resolveOptionThe validation correctly uses
resolveOption
to check the resolved value when enabled is a function, maintaining the same validation logic.
202-205
: LGTM! Consistent comparison of resolved option valuesThe changes correctly compare the resolved values of
enabled
andstaleTime
options usingresolveOption
, ensuring accurate change detection.
216-217
: LGTM! Consistent enabled option comparisonThe change maintains proper change detection for the enabled option using the unified utility.
345-345
: LGTM! Consistent staleTime and refetchInterval resolutionAll three changes correctly use
resolveOption
for computing stale timeout and refetch interval values.Also applies to: 366-366, 377-377
485-489
: LGTM! Clean placeholder data resolutionThe change correctly uses
resolveOption
to handle both direct values and functions for placeholder data, properly passing the previous data and query as arguments when needed.
568-568
: LGTM! Consistent option resolution in result creationBoth changes correctly use
resolveOption
forisEnabled
flag andnotifyOnChangeProps
resolution.Also applies to: 652-652
730-730
: LGTM! Comprehensive update of all conditional checksAll the helper functions have been consistently updated to use
resolveOption
for enabled and staleTime checks, maintaining the correct behavior while using the unified utility.Also applies to: 755-758, 773-773, 784-785
packages/query-core/src/utils.ts (2)
1-2
: LGTM: type-only imports are correct and colocated.The new type imports for Mutation, Query, and FetchOptions are valid here and used below. No issues.
117-160
: Docs/readability: solid API docs and examples.Clear, actionable, and covers both zero-arg and variadic cases. Nice.
export function resolveOption<T, TArgs extends Array<any>>( | ||
value: T | ((...args: TArgs) => T), | ||
...args: TArgs | ||
): T { | ||
return isFunctionVariant(value) ? value(...args) : value | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Critical: resolveOption will eagerly invoke function-typed values (ambiguous when T itself is a function).
If T
can be a function type (e.g., TData
is a function and users provide placeholderData
or initialData
as a function value), resolveOption
will treat that as the “function variant” and call it. This is the exact ambiguity NonFunctionGuard
previously prevented. This change can introduce runtime bugs and unexpected side-effects.
Two concrete fixes (pick one):
- Reintroduce a guard to disallow function-shaped
T
as direct values in this resolver (preferred for safety). - Alternatively, add a second API for “allow function values” and use it only where safe, but that complicates call-sites.
Apply this diff to constrain and keep strong typing (also avoids the any
fallback from the guard):
-export function resolveOption<T, TArgs extends Array<any>>(
- value: T | ((...args: TArgs) => T),
- ...args: TArgs
-): T {
- return isFunctionVariant(value) ? value(...args) : value
-}
+export function resolveOption<T, TArgs extends unknown[] = []>(
+ value: NonFunctionGuard<T> | ((...args: TArgs) => T),
+ ...args: TArgs
+): T {
+ return typeof value === 'function'
+ ? (value as (...args: TArgs) => T)(...args)
+ : (value as T)
+}
Add the missing type where you keep your utility types (preferred: in types.ts
) and import it here:
export type NonFunctionGuard<T> = T extends Function ? never : T
Then extend the types import at the top of this file to include NonFunctionGuard
.
To reduce regressions, please verify we didn’t reintroduce the “function value invoked” bug for options that can carry function-typed data:
🏁 Script executed:
#!/bin/bash
# 1) Find any lingering usage of removed helpers that might mask the issue.
rg -nP -C2 --type=ts '\bresolve(StaleTime|Enabled)\b'
# 2) Locate option types that allow function-typed data (high-risk with generic resolver).
rg -nP -C3 --type=ts '(InitialData|PlaceholderData)\b'
# 3) Grep for call sites of resolveOption with those options.
rg -nP -C3 --type=ts '\bresolveOption\s*\('
# 4) Heuristic: look for placeholderData / initialData assigned a function value (arrow/functions),
# which would be incorrectly invoked if T is function-shaped.
rg -nPU -C2 --type=ts '(placeholderData|initialData)\s*:\s*(\(\s*[^)]*\)\s*=>|function\s*\(|[\w$]+\s*=>)'
Length of output: 160239
Critical: resolveOption eagerly invokes any function values, including those meant as data when T is itself a function type
Currently,
export function resolveOption<T, TArgs extends Array<any>>(
value: T | ((...args: TArgs) => T),
…args: TArgs
): T {
return isFunctionVariant(value) ? value(...args) : value
}
will treat a direct function “value” (when T extends Function) as the callback variant and invoke it, re-introducing the ambiguity that NonFunctionGuard
previously prevented.
Affected call sites include:
- packages/query-core/src/utils.ts (resolveOption, lines 161–166)
- packages/query-core/src/query.ts (initialData resolution)
- packages/query-core/src/queryObserver.ts (placeholderData, enabled, staleTime, refetchInterval, notifyOnChangeProps)
- packages/query-core/src/retryer.ts (retryDelay resolution)
Two concrete fixes (choose one):
- Reintroduce a guard for non-function T values (preferred for type safety):
-export function resolveOption<T, TArgs extends unknown[]>(
+export function resolveOption<T, TArgs extends unknown[] = []>(
- value: T | ((...args: TArgs) => T),
- value: NonFunctionGuard | ((...args: TArgs) => T),
): T {
return typeof value === 'function'
? (value as (...args: TArgs) => T)(…args)
: (value as T)
}
And add to your shared types (e.g. types.ts):
```ts
export type NonFunctionGuard<T> = T extends Function ? never : T
- Alternatively, introduce a second API—e.g.
resolveOptionAllowFunction
—for use cases that legitimately carry function-typed T, and keep the existingresolveOption
safe for all other scenarios.
Both approaches restore strong typing and prevent accidental invocation of function-type data. Please implement one of these fixes across all resolveOption call sites.
🤖 Prompt for AI Agents
In packages/query-core/src/utils.ts around lines 161–166, resolveOption eagerly
invokes any function value which incorrectly calls function-typed data; update
implementation and types to prevent invoking values whose T is itself a
Function. Fix by introducing a NonFunctionGuard<T> = T extends Function ? never
: T in shared types and change resolveOption signature to accept value:
NonFunctionGuard<T> | ((...args: TArgs) => T) so direct function-typed data is
treated as data, not invoked; alternatively implement a second API
resolveOptionAllowFunction for the rare cases that must allow function-typed T,
and then update all affected call sites (packages/query-core/src/query.ts
initialData resolution, packages/query-core/src/queryObserver.ts for
placeholderData, enabled, staleTime, refetchInterval, notifyOnChangeProps, and
packages/query-core/src/retryer.ts retryDelay resolution) to use the safe
resolveOption or the explicit allow-function variant accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/query-core/src/utils.ts (2)
172-172
: Addressed prior note: functionalUpdate now uses resolveOption.This replaces the typeof check with the new utility as requested in earlier review comments.
117-166
: Critical: resolveOption will eagerly invoke function-typed values when T itself is a function (ambiguous semantics).If T is a function type, direct function “values” will be treated as the callback variant and invoked. This is the ambiguity that the former NonFunctionGuard prevented and can lead to runtime bugs and surprising side-effects. This also aligns with prior review feedback.
Fix by constraining T to non-function values for the direct-value branch and defaulting TArgs to unknown[] to improve inference:
-export function resolveOption<T, TArgs extends Array<any>>( - value: T | ((...args: TArgs) => T), - ...args: TArgs -): T { - return isFunctionVariant(value) ? value(...args) : value -} +export function resolveOption<T, TArgs extends unknown[] = []>( + value: NonFunctionGuard<T> | ((...args: TArgs) => T), + ...args: TArgs +): T { + return typeof value === 'function' + ? (value as (...args: TArgs) => T)(...args) + : (value as T) +}Also add the missing utility type and import:
- In types.ts (exported somewhere central):
// Add this near existing utility types export type NonFunctionGuard<T> = T extends Function ? never : T
- Update the import in this file to pull it in (adjust your existing types import):
import type { DefaultError, FetchStatus, MutationKey, MutationStatus, QueryFunction, QueryKey, QueryOptions, + NonFunctionGuard, } from './types'If you need an escape hatch for cases where function-shaped T must be allowed as a direct value, introduce a second API, e.g. resolveOptionAllowFunction, and use it only where intended.
Verification script (locates call sites and checks for the guard’s presence):
#!/bin/bash # 1) Confirm the NonFunctionGuard type exists (or not). rg -nP --type=ts '\btype\s+NonFunctionGuard\b|export\s+type\s+NonFunctionGuard\b' -C2 # 2) List all resolveOption call sites with context to audit ambiguous usages. rg -nP --type=ts '\bresolveOption\s*\(' -C3 # 3) Heuristic: show spots where the resolved T could plausibly be a function-typed value # (look for option names that might carry function-shaped data). rg -nP --type=ts -C2 '(initialData|placeholderData|notifyOnChangeProps|refetchInterval|retryDelay)\b'
🧹 Nitpick comments (1)
packages/query-core/src/utils.ts (1)
80-116
: Prefer unknown[] over Array and avoid leaking any from the guard.Tighten the generics to unknown[] and use unknown instead of any for the guard’s return type. This improves type safety without changing behavior.
Apply:
-function isFunctionVariant<T, TArgs extends Array<any> = []>( - value: T | ((...args: TArgs) => any), -): value is (...args: TArgs) => any { +function isFunctionVariant<T, TArgs extends unknown[] = []>( + value: T | ((...args: TArgs) => unknown), +): value is (...args: TArgs) => unknown { return typeof value === 'function' }Optional: if you foresee external usage for narrowing, consider exporting this guard. Otherwise keeping it internal is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/query-core/src/utils.ts
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/query-core/src/utils.ts (1)
packages/query-core/src/queryObserver.ts (1)
resolveOption
(364-368)
🔇 Additional comments (1)
packages/query-core/src/utils.ts (1)
1-2
: Type-only imports for Mutation/FetchOptions/Query — good move.This prevents runtime cycles and keeps bundles clean. No concerns.
This PR introduces
isFunctionVariant()
andresolveValueOrFunction()
utilities to replace repetitivetypeof === 'function'
checks throughout the codebase, consolidating the common "value or function that computes value" pattern.Problem and Solution
The codebase had scattered implementations of the same pattern across multiple files:
This led to code duplication, inconsistency, and maintenance overhead. We also had specialized versions like
resolveStaleTime()
andresolveEnabled()
that could be generalized.The new utilities provide a clean, generic solution:
While we could inline
typeof value === 'function'
checks, TypeScript's type narrowing doesn't work properly with generic types in complex expressions. TheisFunctionVariant()
type guard provides proper type narrowing that allowsresolveValueOrFunction()
to safely call the function variant. Without it, TypeScript throws errors because it can't guarantee the type safety across the generic boundary.Both utilities support zero-argument functions (
() => T
) and functions with parameters ((...args) => T
), making them applicable to all value-or-function patterns in the codebase.Updated implementations in
query.ts
,queryObserver.ts
, andretryer.ts
for handlinginitialData
,retryDelay
,refetchInterval
, andnotifyOnChangeProps
. These utilities can replace existingresolveStaleTime()
andresolveEnabled()
functions in future iterations.Summary by CodeRabbit
New Features
Refactor
Chores